-
-
Notifications
You must be signed in to change notification settings - Fork 147
WESTMIDLANDS|MAY-2025|SARA TAHIR|Sprint 2/coursework #565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, really good work on this sprint. You've written a lot of really good code, tests, and analysis. I just have 2 small comments within the files for you to have a look at.
module.exports = createLookup; | ||
|
||
/*const pair = [['US','USD'],['CA','CAD']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a major problem here, but remember in a more complex git project not to commit lots of comments of unused code like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I will make sure I dont make it a practice.
Sprint-2/implement/querystring.js
Outdated
if (queryString.startsWith("?")) { // Remove "?" if present at start | ||
queryString = queryString.substring(1); | ||
} | ||
|
||
if (queryString.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given you are checking for empty strings in the later for loop, do you need to also check for an empty string here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the change. You are right, The code would have worked just fine as I am checking again for empty strings later.
Great work on this. You can close this sprint now. |
Learners, PR Template
Self checklist
Changelist
Updated all functions to meet criteria
Wrote tests to see if functions are working correctly
All tests are passing.
Questions
Ask any questions you have for your reviewer.